Preserve type aliases in match | null refinement#19745
Conversation
…ttern) After 'match x with | null -> ... | s -> ...', the type of s loses its alias (e.g. 'string' becomes 'System.String'). These tests lock in the desired behavior; they fail on main and will pass once the fix lands. Refs #19646 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The removeNull helper in TcMatchClause was unconditionally calling
stripTyEqns, which expanded transparent abbreviations like 'string' to
'System.String'. As a result, in:
let test (x: string | null) =
match x with
| null -> null
| s -> s.Replace("a", "b")
the inferred type of 's' was displayed as 'String' instead of 'string'.
Fix: try the cheap path first (replaceNullnessOfTy KnownWithoutNull),
which preserves the abbreviation. If the resulting effective nullness is
already WithoutNull (transparent aliases like 'string' / 'MyStr'), use
that. Otherwise (abbreviations that encode nullness in their RHS, like
'type objnull = obj | null'), fall back to the full stripTyEqns path
introduced by #18852 to keep that regression fixed.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
❗ Release notes required
|
T-Gro
added
AI-Auto-Resolve-Conflicts
Per review feedback: parameterize the three alias preservation cases (string, user alias, Uri alias) into a single Theory with InlineData, and drop the redundant negative test (the case-sensitive substring match on 'string' already rules out 'String'). Refs #19646 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
T-Gro
force-pushed
the
fix/nullness-dropping-alias
branch
from
a34dcfc to
716d47d
Compare
github-actions
Bot
added
the
AI-Tooling-Check-Bypassed
T-Gro
left a comment
T-Gro
left a comment
There was a problem hiding this comment.
Review (self-check)
Clean and well-reasoned fix.
Logic: The two-phase approach (optimistic alias-preserving path → validated via nullnessOfTy → fallback to strip) is correct. When nullness is only in the outer annotation (e.g. string | null), clearing it preserves the alias. When it's baked into the abbreviation RHS (e.g. type objnull = obj | null), the TryEvaluate() check correctly detects the residual nullness and falls back to the prior stripping behavior.
Edge cases covered:
- Direct BCL-alias types (
string→ preserved, not expanded toSystem.String) - User-defined aliases of BCL types (
type MyStr = string→ preserved) - User-defined aliases of reference types (
type MyUri = Uri→ preserved) - Abbreviations with baked-in nullness (
type objnull = obj | null) → correctly falls back (protected by #18488 regression tests)
Test: Using a Theory with InlineData to collapse cases is clean. The test design—intentionally returning s where int is expected to trigger a type-mismatch error that mentions the alias name—is a good indirect way to verify alias preservation.
No concerns on performance (this path runs once per match clause with a null pattern) or correctness.
This comment has been minimized.
This comment has been minimized.
github-project-automation
Bot
moved this from New
to In Progress
in F# Compiler and Tooling
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Note The pull request was not created — a fallback review issue was created instead due to protected file changes: #19823 🤖 LabelOps — Conflict Resolution. Merged What happened: Verified: Build succeeded, all 26 Nullness component tests pass (Release).
|
2 participants
Fixes #19646
After
match x with | null -> … | s -> …, the bindingswas losing its type alias (e.g.stringbecameSystem.String). Now we try to clear nullness on the original type first, preserving the alias; only fall back to stripping abbreviations when nullness is embedded in the abbreviation RHS.